--target-api-url support for migrate repo and org
#1214
Conversation
Unit Test Results797 tests 797 ✅ 22s ⏱️ Results for commit cf1d3d9. ♻️ This comment has been updated with latest results. |
brianaj
changed the title
--target-api-url support for migrate repo and org
There was a problem hiding this comment.
The code looks good.
For the tests, I don't think you need to add the target api url to the command handler tests but rather you need to add a simple test for the handlers themselves to make sure that github api factory is getting invoked with the target api url.
| @@ -23,6 +23,7 @@ public class MigrateRepoCommandHandlerTests | |||
| private const string ADO_REPO = "foo-repo"; | |||
| private const string GITHUB_ORG = "foo-gh-org"; | |||
| private const string GITHUB_REPO = "gh-repo"; | |||
| private const string TARGET_API_URL = "https://api.github.com"; | |||
There was a problem hiding this comment.
I will add a comment just for this line but it applies to all tests for all CLIs.
There is no need to add the target api url here because it's only going to be used inside the Command (not the handler) to create an instance of GithubApi. Instead a new and very simple test needs to be added to the Command tests (e.g. MigrateOrgCommandTests.cs) to make sure if the --target-api-url is provided, the factory is going to receive it.
There was a problem hiding this comment.
The new tests are going to be similar to this one
| @@ -21,6 +21,7 @@ public void Should_Have_Options() | |||
| TestHelpers.VerifyCommandOption(command.Options, "github-source-pat", false); | |||
| TestHelpers.VerifyCommandOption(command.Options, "github-target-pat", false); | |||
| TestHelpers.VerifyCommandOption(command.Options, "verbose", false); | |||
| TestHelpers.VerifyCommandOption(command.Options, "target-api-url", false); | |||
There was a problem hiding this comment.
A similar test to It_Uses_Target_Api_Url_When_Provided also needs to be added for this command too.
There was a problem hiding this comment.
okay will add tomorrow, thanks for the quick feedback!
There was a problem hiding this comment.
@ArinGhazarian added in 833931b, also note I was following the gei spec which originally added this flag, hence why I added test in handler to begin with but thanks for catching it 👍🏾 https://github.com/github/gh-gei/blob/main/src/OctoshiftCLI.Tests/gei/Commands/MigrateRepo/MigrateRepoCommandTests.cs
There was a problem hiding this comment.
Yeah a lot has changed in the CLI since then mostly because the underlying System.CommandLine nuget package pas early beta. Later on as the package became more mature, we changed the way we created the handlers to keep the handles' constructor very simple and easy to test and also keep the concerns as separate as possible so for each command we have the arg, the command and the handler, each one doing a specific thing. IMO the most interesting thing we did was to not inject factories into handlers and delegate the creation of a handler to the command via BuildHanlder and that's where all the creation logic goes.
Co-authored-by: Arin Ghazarian <[email protected]>
| @@ -21,6 +21,7 @@ public void Should_Have_Options() | |||
| TestHelpers.VerifyCommandOption(command.Options, "github-source-pat", false); | |||
| TestHelpers.VerifyCommandOption(command.Options, "github-target-pat", false); | |||
| TestHelpers.VerifyCommandOption(command.Options, "verbose", false); | |||
| TestHelpers.VerifyCommandOption(command.Options, "target-api-url", false); | |||
There was a problem hiding this comment.
Yeah a lot has changed in the CLI since then mostly because the underlying System.CommandLine nuget package pas early beta. Later on as the package became more mature, we changed the way we created the handlers to keep the handles' constructor very simple and easy to test and also keep the concerns as separate as possible so for each command we have the arg, the command and the handler, each one doing a specific thing. IMO the most interesting thing we did was to not inject factories into handlers and delegate the creation of a handler to the command via BuildHanlder and that's where all the creation logic goes.
|
Part of #1094 and https://github.ghe.com/github/octoshift/issues/7956
Adds full support for
migrate-repo(gei already implemented) andmigrate-orgcommands to support--target-api-url.Functional tested for all the above against EU. Details in https://github.ghe.com/github/octoshift/issues/7956#issuecomment-8112051.
ThirdPartyNotices.txt(if applicable)